input: reduce memory allocation for ScriptBuilder #7464
Conversation
guggero
force-pushed
the
script-builder-capacity
branch
from
acf9d10
to
33f27ed
Compare
There was a problem hiding this comment.
Very nice!! Running benchmarks locally and confirming the results,
> benchstat old.prof new.prof
goos: darwin
goarch: arm64
pkg: github.com/lightningnetwork/lnd/input
│ old.prof │ new.prof │
│ sec/op │ sec/op vs base │
ScriptBuilderAlloc-10 15.03m ± 4% 14.44m ± 0% -3.93% (p=0.000 n=10)
│ old.prof │ new.prof │
│ B/op │ B/op vs base │
ScriptBuilderAlloc-10 12.138Mi ± 0% 5.989Mi ± 0% -50.66% (p=0.000 n=10)
│ old.prof │ new.prof │
│ allocs/op │ allocs/op vs base │
ScriptBuilderAlloc-10 94.00k ± 0% 128.00k ± 0% +36.17% (p=0.000 n=10)
This is LGTM once the mod is updated🎉
input/script_utils_test.go
Outdated
|
|
||
| script, err = CommitScriptAnchor(randomPub) | ||
| require.NoError(t, err) | ||
| require.Len(t, script, 40) |
There was a problem hiding this comment.
Got curious so I ran a version without the require.Len, and the results got slightly better with a 52.34% reduction in bytes per operation.
goos: darwin
goarch: arm64
pkg: github.com/lightningnetwork/lnd/input
│ old.prof │ new.prof │
│ sec/op │ sec/op vs base │
ScriptBuilderAlloc-10 8.944m ± 2% 8.882m ± 2% -0.69% (p=0.023 n=10)
│ old.prof │ new.prof │
│ B/op │ B/op vs base │
ScriptBuilderAlloc-10 11.749Mi ± 0% 5.600Mi ± 0% -52.34% (p=0.000 n=10)
│ old.prof │ new.prof │
│ allocs/op │ allocs/op vs base │
ScriptBuilderAlloc-10 77.00k ± 0% 111.00k ± 0% +44.16% (p=0.000 n=10)
There was a problem hiding this comment.
Did you just run it like this?
_, err = WitnessScriptHash(dummyData)
require.NoError(t, err)
I think the compiler just optimizes away the return value, since it's not used. Could that explain the further 52% reduction?
There was a problem hiding this comment.
I think require.Len needs to put the value on the stack since it's referenced outside the function. Validated with a dummy test, which showed no alloc,
script, err = CommitScriptAnchor(randomPub)
require.NoError(t, err)
script[0] = 1 // make a dummy usage of the returned valueThere was a problem hiding this comment.
Okay, I refactored the test a little bit to not use require inside the benchmark. Not sure if this is any better.
guggero
force-pushed
the
script-builder-capacity
branch
from
33f27ed
to
3352b9e
Compare
input/script_utils_test.go
Outdated
|
|
||
| script, err = CommitScriptAnchor(randomPub) | ||
| require.NoError(t, err) | ||
| require.Len(t, script, 40) |
There was a problem hiding this comment.
I think require.Len needs to put the value on the stack since it's referenced outside the function. Validated with a dummy test, which showed no alloc,
script, err = CommitScriptAnchor(randomPub)
require.NoError(t, err)
script[0] = 1 // make a dummy usage of the returned value
guggero
force-pushed
the
script-builder-capacity
branch
from
3352b9e
to
791e02f
Compare
guggero
force-pushed
the
script-builder-capacity
branch
from
791e02f
to
f246161
Compare
|
Changed the base branch to |
guggero
force-pushed
the
script-builder-capacity
branch
3 times, most recently
from
92a7d60
to
b2ce7ff
Compare
|
@ffranr: review reminder |
The default allocation of 500 bytes for the script that is used in NewScriptBuilder is way too much for most of our scripts. With the new functional option we can tune the allocation to exactly what we need.
guggero
force-pushed
the
script-builder-capacity
branch
from
b2ce7ff
to
26254d0
Compare
Depends on btcsuite/btcd#1954.
I got this heap profile from a user and noticed the
NewScriptBuildershowing up with massive allocations:It turns out that function allocates 500 bytes of memory for each script, even if we only end up using about 20-140 bytes for most of our scripts.
I added a new functional option to specify the initial allocation and used that for all our scripts.
The difference is quite significant, it looks like we can save almost 50% of allocation space.